Coverity CID 1122367 Dereference after null check Either the check against null is unnecessary, or there may be a null pointer dereference. In nsMenuPopupFrame::​CreateWidgetForView(nsView *): Pointer is checked against null but then dereferenced anyway 6. var_compare_op: Comparing this->mContent to null implies that this->mContent might be null. 277 if (mContent && widgetData.mNoAutoHide) { 278 if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar, 279 nsGkAtoms::normal, eCaseMatters)) { 280 widgetData.mBorderStyle = eBorderStyle_title; 281 282 mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, title); 283 284 if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::close, 285 nsGkAtoms::_true, eCaseMatters)) { 286 widgetData.mBorderStyle = 287 static_cast<enum nsBorderStyle>(widgetData.mBorderStyle | eBorderStyle_close); 288 } 289 } 290 } 291 292 nsTransparencyMode mode = nsLayoutUtils::GetFrameTransparency(this, this); 7. identity_transfer: Member function call this->GetContent() returns field mContent. [show details] CID 1122367 (#1 of 1): Dereference after null check (FORWARD_NULL)8. var_deref_model: Passing null pointer this->GetContent() to GetParent, which dereferences it. [show details]

We should remove the mContent null-check and replace any mContent with GetContent(). http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/xul/nsMenuPopupFrame.cpp#252

I would like to work on this. Please assign to me. Thanks

Only in nsMenuPopupFrame.cpp file? (In reply to Mats Palmgren (:mats) from comment #1) > We should remove the mContent null-check and replace any mContent with > GetContent(). > http://searchfox.org/mozilla-central/rev/ > abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/xul/nsMenuPopupFrame.cpp#252

Yeah, it's probably not worth doing en masse, but I figured we might as well fix that here while touching this code.

So this kind of LOC: 'if (mContent && widgetData.mNoAutoHide) { if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar, nsGkAtoms::normal, eCaseMatters)) ' will become 'if (GetContent() && widgetData.mNoAutoHide) { if (GetContent().AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar, nsGkAtoms::normal, eCaseMatters)) ' Please correct me If I am doing wrong? (In reply to Mats Palmgren (:mats) from comment #1) > We should remove the mContent null-check and replace any mContent with > GetContent(). > http://searchfox.org/mozilla-central/rev/ > abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/xul/nsMenuPopupFrame.cpp#252

The important bit is to remove the null-check of mContent in the first if-condition, which is what Coverity complained about. But yeah, all other instances of mContent should become GetContent() + fix the indentation.

Please review. Thanks

Comment on attachment 8964202 [details] [diff] [review] Patch_Bug1361602 GetContent() return a pointer, so "GetContent()." won't compile. Also, you forgot "()" in a few places, and you didn't fix the indentation as requested. It's probably a good idea if you setup a local build and make sure your changes compiles before requesting review: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build Thanks!

Please review. './mach xpcshell-test services/sync/tests/unit' -------------------------------------------------- xpcshell ~~~~~~~~ Ran 105 checks (105 tests) Expected results: 103 Skipped: 2 tests OK ---------------------------------------------------

Comment on attachment 8964434 [details] [diff] [review] PatchV2_Bug1361602 I don't think this needs two reviewers. Make sure to fix the indentation though + GetContent()->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::remote, The second line should line up with the parenthesis.

Please review.

Comment on attachment 8964689 [details] [diff] [review] PatchV3_Bug1361602 I don't see anything in this patch that corresponds to what the commit message says, so presumably somebody already fixed that part? So you should change the commit message to describe what you're actually changing here. AFAICT, this patch only replaces mContent with GetContent(), which is an idempotent change, so you might want to include "(idempotent patch)" at the end of the commit message to make that clear. >- mContent->AsElement()->AttrValueIs(kNameSpaceID_None, >- nsGkAtoms::remote, >- nsGkAtoms::_true, eIgnoreCase)); >+ GetContent()->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::remote, nsGkAtoms::_true, eIgnoreCase)); I don't think we should change the line-breaking like this. In general, it's a good idea to follow the existing formatting, i.e. function arguments that are lined up on separate lines should continue to do so unless there's a reason not to. Also, as a rule-of-thumb, we try to keep lines within 80 columns, unless it's motivated for readability reasons to make the lines longer (which it rarely is). So please keep the general formatting as is, except for adding a few indentation spaces so that arguments still align as noted in comment 10.